Some changes occurred in src/doc/unstable-book/src/compiler-flags/sanitizer.md

cc @rust-lang/project-exploit-mitigations, @rcvalle

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

☔ The latest upstream changes (presumably #134395) made this pull request unmergeable. Please resolve the merge conflicts.

r? lang

This probably needs a new lang FCP since the old one is probably outdated (the implementation of naked function has changed signficantly).

Thanks for the thorough report @folkertdev!

@rfcbot fcp merge

Team member @tmandry has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• maybe-unsafe-attribute resolved by Stabilize naked_functions #134213 (comment)
• mention-nop-padding-and-ud2 resolved by Stabilize naked_functions #134213 (comment)
• question-about-noreturn resolved by Stabilize naked_functions #134213 (comment)
• question-about-rust-abi resolved by Stabilize naked_functions #134213 (comment)
• question-about-unwind resolved by Stabilize naked_functions #134213 (comment)
• what-to-do-for-target-feature resolved by Stabilize naked_functions #134213 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

Actually, @rust-lang/libs-api does this need your FCP? I think the path of core::arch::naked_asm! is the only part that might.

catching up to some comments

Of course, we could argue that, "well, you can't actually create unsafety without the inner unsafe { .. }", but still.

Yes that is what I would argue :)

from the edition guide

Rust 1.82 added the ability in all editions to mark certain attributes as unsafe to indicate that they have soundness requirements that must be upheld

There are no soundness requirements to a naked function itself. The requirements are on the naked_asm!, which is already covered by an unsafe block. What additional value does making the attribute itself unsafe have?

Also, just to make sure I understand correctly, the following would not compile, right?

#[naked]
pub extern "sysv64" fn f() {}

It is obviously unsound, without actually containing an unsafe block, as calling this function would start executing padding bytes, so it should be disallowed.

Correct, that is tested here:

We can map our target feature names to the ones LLVM uses easily, we already do that. Is that not enough?

Sadly not. LLVM target features are also just made-up names that don't always map to the names that the assembler uses. E.g. the arm trustzone feature is known in the assembler as sec (for security, I guess). Most targets behave much more reasonably but some clearly don't.

Is there any reason for not just moving #[naked] with a #[target_feature] to a separate feature gate and stabilize naked_function without this for now?

That is my proposed course of action. So far nobody seems to really be against this approach, so if things stay that way I'll make a PR.

LLVM target features are also just made-up names that don't always map to the names that the assembler uses.

And I guess we can't send the naked function "through" LLVM to let it do the translation?

That might be a good feature request for them...

Is there any reason for not just moving #[naked] with a #[target_feature] to a separate feature gate and stabilize naked_function without this for now?

Sounds good to me.

And I guess we can't send the naked function "through" LLVM to let it do the translation?

No that was the original design, but it was rejected for various reasons:

• LLVM would insert additional instructions in some cases (or at least did not guarantee that it would not)
• we have non-LLVM backends that would need to handle this logic too

Some further context from the tracking issue:

#90957 (comment)
#90957 (comment)

we have non-LLVM backends that would need to handle this logic too

As they should. They have to deal with target feature names anyway for regular code.

Anyway, I am mostly clueless here, just poking a bit in the dark to feel out the design space. @Amanieu knows a lot better than me how to make inline assembly work well. :)

☔ The latest upstream changes (presumably #138450) made this pull request unmergeable. Please resolve the merge conflicts.

☔ The latest upstream changes (presumably #138791) made this pull request unmergeable. Please resolve the merge conflicts.

The #[target_feature] attribute on naked functions is now separately guarded by the naked_functions_target_feature feature. I think that should resolve the concern here around target features.

tracking issue: #138568
PR: #138570

Looking at the other concerns, noreturn has been added to the historical notes in the stabilization report, and padding/ud2 are now mentioned there too. Unless there is something else, I think these are also resolved?

The current remaining concerns are then:

• question-about-rust-abi: looks like the consensus is that using the rust abi should not be explicitly disallowed, even though it is extremely unsafe to use it? Note that to use it, you must use extern "Rust" fn, a standard fn triggers a lint.
• maybe-unsafe-attribute: it's been discussed. I don't personally see much value in it, but if anyone wants to really argue in favor, please do?

As they should. They have to deal with target feature names anyway for regular code.

Ah, I misunderstood this earlier. What I meant is that we could have used llvm's native support for naked functions, which has the downsides that I and Amanieu mentioned.

I think you propose to still use global assembly, but somehow inform the backend of the target features. That would be neat, but can't currently be done even in LLVM.

Is there a practical reason to allow extern "Rust" fn?

I think unless there is a specific need for it it would be best to disallow it (From an mostly outsider perspective, I'm not really familiar with the state of Rust ABI and the current state of naked functions).
From what I understand Rust ABI is not stable, so I don't expect it to be very useful but I might be missing something?

Linting on extern "Rust" fn seems reasonable to me, but I don't think we should forbid it. It's useful for experimentation and possibly internal tests, if nothing else.

The #[target_feature] attribute on naked functions is now separately guarded by the naked_functions_target_feature feature. I think that should resolve the concern here around target features.

tracking issue: #138568
PR: #138570

Sounds right. Thanks for filing the new tracking issue.

@rfcbot resolve what-to-do-for-target-feature

Looking at the other concerns, noreturn has been added to the historical notes in the stabilization report, and padding/ud2 are now mentioned there too. Unless there is something else, I think these are also resolved?

They are. Thanks for adding these notes. The rationales and outcomes there seem right to me.

@rfcbot resolve question-about-noreturn
@rfcbot resolve mention-nop-padding-and-ud2

The current remaining concerns are then...

These probably need to be discussed with the team. We'll take them up in a lang call.

On the ud2 point: If debug mode rustc always arranged that functions were laid out such that there just so happened to be a ud2 after every naked functions, that would still be fine, right?

Not requiring that certainly makes sense to me, though, especially for opt-level=z.

OK. On the lang call today, we reviewed all of the concerns resolved above, and we worked through the two remaining items. Here's what we want to do.

One, we don't want to stabilize naked functions with the "Rust" ABI as part of this stabilization. We'd like that (and all rustic ABIs) carved off into a separate feature flag and tracking issue. Then, if we find there's some use case for this that doesn't rely on details to which we're not willing to commit, we can later consider that on its own.

One reason for this is caution due to Hyrum's law:

With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended on by somebody.

Because of the nature of naked functions, it's difficult to use them without relying on details of the ABI, and we don't want to later have this conversation when making changes to the Rust ABI:

Bob: Wait, you can't change that! I've written a bunch of naked functions that are possibly broken now.

Alice: We didn't guarantee any details about the Rust ABI though.

Bob: Then why did you stabilize naked functions for the Rust ABI? There's no way to use them without relying on those details.

By contrast, someone having to reach for nightly to do this sends the right signal.

Two, we want to move the unsafe out of the body and into the attribute. That is, naked will be an unsafe attribute, and the naked_asm! macro -- which can only appear within a #[unsafe(naked)] function (and is required to do so and is the only thing that can appear within such a function) -- does not need to be wrapped in an unsafe { .. } block within the function.

We had a good long and hearty discussion about this one looking at it from every angle. The deciding factor here is in thinking about the proper scope of the unsafe obligation being discharged. What we're really trying to say is in fact:

unsafe {
    #[naked]
    extern "sysv64" fn f() {
        naked_asm! {
            ..
        }
    }
}

That is, the author of the function needs to prove certain properties about the function as a whole, inclusive of and affected by that naked attribute. In particular, one needs to prove it's upholding its signature. Naked functions are particularly special here, as the naked attribute is removing the function prelude, which is obviously unsafe, and the code in the function body then has to work, according to the stated ABI, to satisfy the signature all while not doing UB. In this way, the naked attribute and the naked_asm! block essentially form one atomic unit of unsafe obligations that must be discharged.

Therefore it makes sense to move the unsafe outward to the attribute and say that what we mean in doing that is to extend the scope of discharge to the entire function, exactly as if we had item-level unsafe, as above. So we'll write:

#[unsafe(naked)]
extern "sysv64" fn f() {
    naked_asm! {
        ..
    }
}

Pleasingly, this also avoids some rightward drift.

I'll go ahead and resolve the concerns as we've answered the questions here.

@rfcbot resolve maybe-unsafe-attribute
@rfcbot resolve question-about-rust-abi

Please update the PR (and the Reference PR) accordingly and hand it back to me.

@rustbot author

🔔 This is now entering its final comment period, as per the review above. 🔔

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.